Make certain panicky stdlib functions behave better under panic_immediate_abort #91737
Conversation
|
(rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
Looks reasonable to me. One way to write a test for this would be to build an executable that calls certain functions, look at the (demangled) symbols it references, and verify that the formatting machinery isn't present. But I think such tests could go in another PR. @bors r+ |
|
📌 Commit 917dafc has been approved by |
|
🌲 The tree is currently closed for pull requests below priority 100. This pull request will be tested once the tree is reopened. |
bors
added
S-waiting-on-bors
Do we have support for (I assume this requires the Cargo |
| #[cfg_attr(not(feature = "panic_immediate_abort"), inline(never))] | ||
| #[cfg_attr(feature = "panic_immediate_abort", inline)] | ||
| #[cold] |
There was a problem hiding this comment.
Does #[inline] have any effect when it's also #[cold]?
There was a problem hiding this comment.
No idea! We could gate that attribute too, I guess
| #[inline(never)] | ||
| #[cold] | ||
| #[track_caller] | ||
| fn unwrap_failed(msg: &str, error: &dyn fmt::Debug) -> ! { | ||
| panic!("{}: {:?}", msg, error) | ||
| } | ||
|
|
||
| // This is a separate function to avoid constructing a `dyn Debug` | ||
| // that gets immediately thrown away, since vtables don't get cleaned up | ||
| // by dead code elimination if a trait object is constructed even if it goes |
There was a problem hiding this comment.
Is this a limitation of LLVM, or is it something that could be addressed on our end?
There was a problem hiding this comment.
I don't think this can be addressed on our end because it's something that has to be run after the rest of the optimizations
There was a problem hiding this comment.
Isn't #[inline] enough? Would be useful to look at the optimized LLVM IR that still contains the vtable (for something that doesn't really have any code other than what's rooted by the vtable), and take stock of what's exported from the LLVM module.
I am pretty sure LLVM can handle removing unused fn params, but if a global (static/fn) isn't marked private/internal/etc. to avoid export, then the ABI demands its signature be unchanged (and potentially even non-inlining interprocedural optimizations might be less effective, but I'm not sure if LLVM considers an exported definition to be opaque in any way, I kinda doubt it).
There was a problem hiding this comment.
@eddyb In my experience LLVM is eager to keep around vtable specs that are never used. I think it treats the construction of a dyn and its use differently and if one goes away it doesn't necessarily figure out the other should too.
…askrgr Rollup of 8 pull requests Successful merges: - rust-lang#90081 (Make `intrinsics::write_bytes` const) - rust-lang#91643 (asm: Allow using r9 (ARM) and x18 (AArch64) if they are not reserved by the current target) - rust-lang#91737 (Make certain panicky stdlib functions behave better under panic_immediate_abort) - rust-lang#91750 (rustdoc: Add regression test for Iterator as notable trait on &T) - rust-lang#91764 (Do not ICE when suggesting elided lifetimes on non-existent spans.) - rust-lang#91780 (Remove hir::Node::hir_id.) - rust-lang#91797 (Fix zero-sized reference to deallocated memory) - rust-lang#91806 (Make `Unique`s methods `const`) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
The stdlib has a
panic_immediate_abortfeature that turns panics into immediate aborts, without any formatting/display logic. This feature was introduced primarily for codesize-constrained situations.Unfortunately, this win doesn't quite propagate to
Result::expect()andResult::unwrap(), while the formatting machinery is reduced,expect()andunwrap()both callunwrap_failed("msg", &err)which has a signature offn unwrap_failed(msg: &str, error: &dyn fmt::Debug)and is#[inline(never)]. This means thatunwrap_failedwill unconditionally construct adyn Debugtrait object even though the object is never used in the function.Constructing a trait object (even if you never call a method on it!) forces rust to include the vtable and any dependencies. This means that in
panic_immediate_abortmode, calling expect/unwrap on a Result will pull in a whole bunch of formatting code for the error type even if it's completely unused.This PR swaps out the function with one that won't require a trait object such that it won't force the inclusion of vtables in the code. It also gates off
#[inline(never)]in a bunch of other places where allowing the inlining of an abort may be useful (this kind of thing is already done elsewhere in the stdlib).I don't know how to write a test for this; we don't really seem to have any tests for
panic_immediate_abortanyway so perhaps it's fine as is.